Various bug fixes throughout the code base #171
Conversation
iCharlesHu
requested review from
cthielen,
itingliu,
jakepetroules and
parkera
iCharlesHu
force-pushed
the
charles/bug-fixes
branch
from
971281a to
397028c
Compare
iCharlesHu
changed the title
|
|
| int flags = fcntl(fd, F_GETFD); | ||
| if (flags >= 0) { | ||
| // Set FD_CLOEXEC on every open fd so they are closed after exec() | ||
| fcntl(fd, F_SETFD, flags | FD_CLOEXEC); |
There was a problem hiding this comment.
If you're doing a syscall anyway, why don't you just close it?
There was a problem hiding this comment.
For two reasons:
- I've searched around (close vs setting FD_CLOEXEC) and it seems in general
FD_CLOEXECis preferred for this situation:
https://utcc.utoronto.ca/~cks/space/blog/unix/ForkFDsAndRaces
- A more practical reason is I don't think we can safely
close()while traversing/dev/fdbeforeclose()modifies that directory.
There was a problem hiding this comment.
I've searched around (close vs setting FD_CLOEXEC) and it seems in general FD_CLOEXEC is preferred for this situation:
Both of those articles I think are really talking about the pre-fork case. I think in our specific situation where we've already called fork (and thus have exactly one thread and no concurrency considerations), and we can guarantee that we will not call fork again, and that we will call exec imminently, there's no practical difference (that I can see).
A more practical reason is I don't think we can safely close() while traversing /dev/fd before close() modifies that directory.
This seems like a great point though!
There was a problem hiding this comment.
A more practical reason is I don't think we can safely close() while traversing /dev/fd before close() modifies that directory.
That should not happen and doesn't happen in the original code from swift-sdk-generator. How the original code works is:
- Get the maximum fd
- Loop from 3 to maximum fd and close
That's completely fine. What is not fine (but that's not what the original code does) is to close whilst iterating of course.
| for (int fd = STDERR_FILENO + 1; fd <= highest_open_fd; fd++) { | ||
| // We must NOT close pipefd[1] for writing errors | ||
| if (fd != pipefd[1]) { | ||
| close(fd); |
There was a problem hiding this comment.
close feels better here and will be cheaper overall. Otherwise you need to do N syscalls to mark as CLOEXEC and later on there's more work necessary to close them. Why not just close?
There was a problem hiding this comment.
Ditto to above: it seems the general consensus is to use FD_CLOEXEC instead of close(). We would have to issue N close() calls here, so without close_range, it doesn't really save much.
- Instead of calling `[UInt8].removeFirst` on `LineSequence`’s `underlyingBuffer`, we now use an index-based approach. This change improves overall streaming performance by eliminating the `O(n)` `.removeFirst()` operation. - The `safelyCloseMultiple` function has been updated to handle cases where the same `IODescriptor` is passed to multiple input/outputs. - On Linux, `_highest_possibly_open_fd_dir_linux()` has been updated to `_close_open_fd_linux()` to more efficiently emulate `POSIX_SPAWN_CLOEXEC_DEFAULT`.
iCharlesHu
force-pushed
the
charles/bug-fixes
branch
from
397028c to
90e0875
Compare
[UInt8].removeFirstonLineSequence’sunderlyingBuffer, we now use an index-based approach. This change improves overall streaming performance by eliminating theO(n).removeFirst()operation.safelyCloseMultiplefunction has been updated to handle cases where the sameIODescriptoris passed to multiple input/outputs._highest_possibly_open_fd_dir_linux()has been updated to_close_open_fd_linux()to more efficiently emulatePOSIX_SPAWN_CLOEXEC_DEFAULT.Resolves: #165, #160, #159